Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rustdoc: Use correct def_id for doctree::Import #79751

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Rustdoc: Use correct def_id for doctree::Import #79751

merged 1 commit into from
Dec 7, 2020

Conversation

aDotInTheVoid
Copy link
Member

The default overwrites the crate root, which crashes rustdoc-json.

While investigating this, It turns out somehow, some items are being documented twice. I'm not sure how this is happening but for now, we just make sure they were the same if they have the same id.

Zulip descussion

Bless script (Once this is more pollished I'll submit it)

r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2020
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 5, 2020
jyn514
jyn514 reviewed Dec 5, 2020
View reviewed changes
src/librustdoc/json/mod.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Dec 7, 2020

@bors r+

I have a nagging feeling this is fixing the symptom instead of the underlying bug, since the JSON module is still getting tricked into overwriting the original item with the import, but this seems strictly better than the current ICE so I'm ok with merging it for now.

@bors
Copy link
Contributor

bors commented Dec 7, 2020

📌 Commit c254a15 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2020
@bors
Copy link
Contributor

bors commented Dec 7, 2020

⌛ Testing commit c254a15 with merge 34aaff8c6343edf57d1716b9119e3e11770da033...

@bors
Copy link
Contributor

bors commented Dec 7, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 7, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 7, 2020

Cargotest failed while testing xsv because proptest's test cases are randomized. cc @rust-lang/infra - I thought we got rid of randomized tests at some point? Did we miss some?

cc @BurntSushi, you may be interested in fixing the xsv bug (assuming it hasn't already been fixed on master, I think cargotest is pretty out of date): https://github.com/rust-lang-ci/rust/runs/1511131529#step:24:8300

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2020
@bors
Copy link
Contributor

bors commented Dec 7, 2020

⌛ Testing commit c254a15 with merge afa995b...

@BurntSushi
Copy link
Member

@jyn514 I think that bug (or test) should be fixed on master. Are y'all using master or the latest release? The latest release is pretty old at this point and I'm not sure when I'll get the next one out.

@jyn514
Copy link
Member

jyn514 commented Dec 7, 2020

We appear to be using a pinned commit from 2017 🤦

rust/src/tools/cargotest/main.rs

Line 39 in b5ff9c3

sha: "66956b6bfd62d6ac767a6b6499c982eae20a2c9f",

I'll see about updating that.

@jyn514 jyn514 mentioned this pull request Dec 7, 2020
Merged
@aDotInTheVoid
Copy link
Member Author

I have a nagging feeling this is fixing the symptom instead of the underlying bug, since the JSON module is still getting tricked into overwriting the original item with the import

Theres 2 things going on here, that caused 2 items with the same ID to be inserted, only one of which this fixes

  1. Imports were given the CRATE_DEF_INDEX instead of their real ID. This is fixed
  2. Items were being documented twice. This is not fixed

I've added a check that the item removed from the map is the same as what was put, so we ICE if case 1 comes back. For now, case 2 is silently ignored, but this will be changed when it is fixed.

@jyn514
Copy link
Member

jyn514 commented Dec 7, 2020

Imports were given the CRATE_DEF_INDEX instead of their real ID. This is fixed

Well, but they're being given the ID of the item, right, not the id of the import? I think that's where the duplicates come from, it's being stored for both the import and the original item. Consider

mod x {
  pub struct S;
}
pub use x::S;

Right now I think it will insert S into the map twice: even though the item is unique, the places it is accessible are not.

@bors
Copy link
Contributor

bors commented Dec 7, 2020

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing afa995b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 7, 2020
@bors bors merged commit afa995b into rust-lang:master Dec 7, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 7, 2020
@aDotInTheVoid
Copy link
Member Author

Well, but they're being given the ID of the item, right, not the id of the import?

We have id 0:7, which is the import of id 0:5, so they do have seperate IDs

@aDotInTheVoid aDotInTheVoid mentioned this pull request Dec 7, 2020
Closed
tmandry added a commit to tmandry/rust that referenced this pull request Dec 9, 2020
@tmandry
Rollup merge of rust-lang#79803 - jyn514:xsv, r=Mark-Simulacrum
0994f35 
Update xsv to prevent random CI failures

This fixes occasional proptest failures due to a bug in xsv, which
aren't related to bugs in the rust compiler.

See rust-lang#79751 (comment) for context.
@aDotInTheVoid aDotInTheVoid mentioned this pull request Jan 3, 2021
Closed
@aDotInTheVoid aDotInTheVoid deleted the json-true-idx branch January 6, 2021 12:27
@aDotInTheVoid
Copy link
Member Author

@rustbot modify labels +A-rustdoc-json

@rustbot rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@jyn514 jyn514

Labels
A-rustdoc-json Area: Rustdoc JSON backend merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.50.0
Development

Successfully merging this pull request may close these issues.
7 participants
@aDotInTheVoid @jyn514 @bors @BurntSushi @GuillaumeGomez @rust-highfive @rustbot